staticaddr: support multiple receive and change addresses - #1218
staticaddr: support multiple receive and change addresses#1218hieblmi wants to merge 22 commits into
Conversation
a1b5ec5 to
c74ac6c
Compare
96317e8 to
7a46f7e
Compare
Reserve separate key families for static receive and change addresses. This keeps derived keys out of the legacy static-address and HTLC key streams.
Associate every deposit with the static address parameters that created it. This lets restored deposits recover the correct script and signing keys instead of assuming the legacy root address.
ee1bbba to
5b4bd5b
Compare
5b4bd5b to
c49a43e
Compare
|
@starius: review reminder |
Create receive and change addresses from locally derived client keys while reusing the server key and expiry from the legacy seed. Persist, import, and activate each script before returning it to callers. Rebuild the active address index on startup and serialize issuance without blocking address reads. Import only scripts missing from lnd, and accept duplicate-import errors only when they identify the expected Taproot output key.
Look up each newly discovered wallet UTXO by script and persist the matching active-address parameters on the deposit. Reject unknown scripts before allocating the timeout sweep address. Use the per-deposit parameters when constructing the FSM, sign descriptor, and unilateral expiry sweep so derived-address recovery uses its owning script and key.
Register timeout-sweep confirmations by destination script instead of the originally published txid. This lets recovery detect an RBF replacement after restart with a stale txid.
Construct each cooperative MuSig2 session from the address parameters stored on its deposit. This prepares loop-ins and withdrawals to sign inputs belonging to different derived static addresses. Clean up sessions created before a later setup failure. Reject duplicate deposit outpoints to avoid leaking signer state. Validate transaction inputs, session handles, and nonce counts before signing so malformed responses fail before any signer operation.
Map every selected outpoint to the static address descriptor that derived its deposit and include those descriptors in loop-in requests. This lets the server validate mixed-address inputs independently of request order.
Include the derivation key for every withdrawal input in the server request. This lets the server validate and sign withdrawals that combine deposits from multiple derived addresses.
Join restored deposits with their persisted static-address rows and hydrate pre-migration deposits from the legacy root address. Return complete ownership metadata from deposit reads and use each deposit's own expiry during loop-in selection.
Associate fractional loop-ins with their operation-specific static change address so recovery restores the descriptor needed to reconstruct signed transactions. Backfill legacy fractional swaps to the original address.
Create a fresh static address for fractional loop-in change and send its descriptor to the server. Reconstruct signed HTLCs with the persisted parameters and verify cooperative batch change by output script.
Multi-address loop-ins sign and construct transactions from the parameters attached to each deposit and their dedicated change address. The legacy root address fields therefore became write-only, but populating them could still abort signing, sweep handling, or recovery when the root lookup failed. Remove those fields and lookups, select the FSM from the protocol version persisted with the swap, and set that version before constructing new state machines. Keep the root-parameter lookup used by autoloop expiry calculation and add regression coverage for recovery and unsupported persisted versions.
Create a fresh static address for partial-withdrawal change and identify it in the confirmed transaction through its active change-family script, without assuming output order or count. Record withdrawn and change amounts by script identity. Keep all withdrawal outputs in the PSBT without separate signing metadata while preserving full-withdrawal behavior.
Let loop static deposit create and fund a fresh receive address through lnd SendCoins. Validate funding arguments before address creation and require explicit confirmation unless --force is set, including for non-interactive and first-use deposits. Allow NewStaticAddress RPC callers to fund a requested existing static address by resolving it through the active script index. Expose the nested request through the client RPC, require swap:execute permission, and cover the CLI new-address and daemon existing-address funding paths. Regenerate RPC and CLI documentation.
Include the owning static address in every deposit RPC response and CLI listing. Users can distinguish deposits created by different receive and change addresses without reconstructing scripts externally. Calculate blocks until expiry from each deposit owner instead of the legacy root address, and reject deposits whose owning parameters are missing. Centralize deposit response conversion and update generated RPC artifacts, regression coverage, and command replay fixtures.
The CLI previously recognized an uninitialized static-address seed by searching arbitrary gRPC error text. Any wrapping or wording change could suppress the L402 backup warning before a user funded a newly derived address. Map ErrNoStaticAddress to codes.NotFound at the RPC boundary and classify that status in the CLI. Retain compatibility with older daemons only for an exact Unknown-status message, avoiding the broad substring match, and cover both sides with regression tests.
A static-address account can now receive deposits across multiple derived addresses, so the singular summary field can no longer describe the current receive address. Removing or repurposing field 1 would break existing clients. Keep the wire value as the legacy/root derivation address, formally deprecate it, document the expiry as the shared CSV delay, and direct CLI users to derive a fresh receive address. Rename the server locals to make the compatibility behavior explicit and regenerate protobuf and Swagger artifacts.
Cover per-deposit address ownership and operation-specific change outputs across the shared SQL persistence boundary. Reconstruct the deposit, loop-in, and withdrawal stores to verify ownership and change metadata survive restart.
The sweep request handler only compared the number of prevouts the server sent against the number of sweep inputs. A list of the right length could still contain duplicate outpoints or reference outpoints the sweep doesn't spend. The prevout fetcher then returns nil for an input and NewTxSigHashes panics on the nil dereference, taking down loopd on a malformed server request. Reject duplicate prevouts while building the prevout map and require a prevout for every sweep input before computing sighashes.
Document fresh receive-address derivation, lazy seed initialization, funding-address lookup hardening, and the swap:execute permission required by address creation. Regenerate the CLI, gRPC, Swagger, and man-page documentation and add feature, breaking-change, and recovery release notes.
c49a43e to
56816b7
Compare
| // TestStaticAddressKeyFamiliesAreDisjoint documents the key-family split used | ||
| // by static-address HTLC, receive and change key derivation. | ||
| func TestStaticAddressKeyFamiliesAreDisjoint(t *testing.T) { | ||
| families := map[int32]string{ |
There was a problem hiding this comment.
I would remove this test. It just tests the consts' values. The values of families are never used.
| // StaticAddressKeyFamily is the legacy static-address key family. It is | ||
| // used for the V0 single static-address key and for static-address HTLC | ||
| // keys. | ||
| StaticAddressKeyFamily = int32(42060) |
There was a problem hiding this comment.
Can we rename it to StaticSingleAddressKeyFamily?
|
|
||
| // StaticAddressChangeKeyFamily is the key family used to generate | ||
| // static-address change outputs. | ||
| StaticAddressChangeKeyFamily = int32(42062) |
There was a problem hiding this comment.
It is used as a change for both single-address and multi-address. Can we add this note explicitly to the godoc?
| return nil, fmt.Errorf("missing static address "+ | ||
| "parameters for %v", u.OutPoint) |
There was a problem hiding this comment.
Should we also include address or pkscript to the error message?
| } | ||
|
|
||
| // toAddressParameters transforms a database representation of a static address | ||
| // to an AddressParameters struct. |
There was a problem hiding this comment.
There is no such struct type AddressParameters, but I propose to actually call it that (see another comment).
| // GetAllStaticAddresses retrieves all static addresses from the store. | ||
| GetAllStaticAddresses(ctx context.Context) ([]*script.Parameters, | ||
| error) | ||
| GetAllStaticAddresses(ctx context.Context) ([]*Parameters, error) |
There was a problem hiding this comment.
I don't think such a method is a good API in multi-address world. The number of static addresses in a wallet may be huge. We should avoid loading them all at once to memory. Instead we should do pagination. And use it only rarely anyway, avoiding doing it regularly. Ideally if we do it once when the daemon starts and then maintain in memory some representation sufficient to operate.
|
|
||
| cfg *ManagerConfig | ||
| cfg *ManagerConfig | ||
| issuanceMu sync.Mutex |
There was a problem hiding this comment.
This mutex currently covers much more than in-memory state. EnsureStaticAddressSeed holds it while loading addresses from the DB, calling the server, deriving an lnd key, inserting and re-reading DB records, and importing Taproot scripts into the wallet. newDerivedAddress similarly holds it across key derivation, DB operations, and wallet import. A slow or stuck dependency therefore blocks every receive- and change-address request, and callers waiting on sync.Mutex.Lock cannot honor context cancellation.
I think only root-address creation/recovery requires serialization, so concurrent first callers cannot create different roots. Once the root exists, derived address issuances are independent and should not be globally serialized. Could we replace issuanceMu with a retryable, context-aware single-flight operation in EnsureStaticAddressSeed, with waiters selecting on their own ctx.Done(), and remove it from newDerivedAddress? sync.Once would not be suitable because transient initialization failures must remain retryable.
Separately, now that the type has multiple synchronization concerns, the embedded sync.Mutex should become a named private field:
activeMu sync.RWMutex // guards activeStaticAddressesEmbedding promotes Lock and Unlock as public Manager methods and no longer makes it clear which state the mutex protects. Most map access paths (GetParameters, legacyParameters, and address filtering in ListUnspentRaw) are reads, so RWMutex also avoids unnecessarily excluding independent readers. The O(N) scan/copy in those latter methods should ideally be reduced separately, but at minimum the lock ownership and scope should be explicit.
| "/looprpc.SwapClient/NewStaticAddress": {{ | ||
| Entity: "swap", | ||
| Action: "read", | ||
| Action: "execute", |
There was a problem hiding this comment.
This grants every existing swap:execute + loop:in macaroon access to the expanded NewStaticAddress endpoint. A bearer can submit SendCoinsRequest{SendAll: true} to move the eligible lnd wallet balance into a static address; the same permissions authorize WithdrawDeposits with an arbitrary dest_addr, allowing those funds to be withdrawn externally after confirmation. Please split funding into a separately permissioned RPC, or require a dedicated funding permission that existing execute macaroons do not carry. The release notes should also explicitly describe this authority expansion.
| return nil, fmt.Errorf("static address manager not configured") | ||
| } | ||
|
|
||
| staticAddress, err := s.staticAddressManager.GetTaprootAddress( |
There was a problem hiding this comment.
This reconstructs the Taproot tree and performs MuSig2 key aggregation for every returned deposit. Since rpcDeposit is used by deposit, withdrawal, and swap listings, this adds elliptic-curve work per record. AddressParams.PkScript already contains the canonical P2TR output script, so could we derive or cache the encoded address from it instead? The same applies to ListUnspentDeposits.
| err = validateStaticQuoteDepositsSwappable( | ||
| depositList.FilteredDeposits, staticAddrExpiry, | ||
| currentHeight, | ||
| selectedDeposits, err := s.depositManager.DepositsForOutpoints( |
There was a problem hiding this comment.
This retrieves the selected deposits twice. ListStaticAddressDeposits first loads the complete deposit history, filters and renders the selected records, and performs another height lookup; DepositsForOutpoints then reloads the same selection for expiry validation. Could we use one exact-outpoint live-deposit lookup to validate presence, state, and expiry and calculate the count and value in one pass?
4 participants
This is PR 1 of 3 in the Static Address multi-address stack.
Static Address previously treated one legacy/root address as the owner of every
deposit. This PR introduces fresh receive and operation-specific change
addresses while preserving the address parameters that own each deposit.
Loop-ins and withdrawals can consequently spend deposits received across
multiple derived addresses, with each input signed and proven using its actual
address parameters.
Key Changes
across restarts.
address.
loop-ins and partial withdrawals.
errors for the expected Taproot output key.
of reconstructing and scanning every persisted address.
paying the same destination script cannot finalize a deposit.
loop static depositsupport for creating and optionally funding a freshaddress through lnd
SendCoins.RPC and Compatibility Changes
NewStaticAddressnow derives a fresh receive address when no existing addressis supplied through
send_coins_request.addr. Callers must not assume repeatedrequests are idempotent or return the same address.
Because address creation mutates wallet and database state, the RPC permission
changes from
swap:readtoswap:execute. Operators using custom scopedmacaroons must rebake them accordingly.
StaticAddressSummaryResponse.static_addressremains populated with thelegacy/root address for wire compatibility, but is deprecated and must not be
treated as the current receive address. Call
NewStaticAddressto derive afresh address.
Database and Recovery
The database now records:
Migrations backfill existing records using the legacy/root address. Store
reconstruction restores deposit ownership and loop-in and withdrawal change
metadata after restart.
Stacked PRs
pull/1215)
validates and follows the transactions that actually replace multi-address
withdrawals.
persists the confirmed HTLC output so recovery can rebuild the exact
server-published transaction.
The address startup and lookup hardening previously isolated in
#1214 has been folded into
this PR, and #1214 is now closed.
Testing
go test ./...go vet ./...go test -race ./staticaddr/address ./loopdmake docs-checkgit diff --checkRelease Notes
Release notes document the new address behavior, RPC permission change,
deprecated summary field, migration compatibility, and address lookup
hardening.